-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Add lint clones_into_boxed_slices
#16095
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
|
Lintcheck changes for 1ef6ca9
This comment will be updated if you push new changes |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting lint. It should probably be located inside the methods directory though.
Also, it should check the MSRV to ensure that the proposed fixes are actionable. For example .as_c_str() didn't exist before Rust 1.20.
| let mut sugg = Sugg::hir_with_context(cx, inner, full_span.ctxt(), placeholder, &mut applicability); | ||
|
|
||
| let inner_ty = cx.typeck_results().expr_ty(inner); | ||
| let mut ref_count = count_refs(inner_ty); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use peel_and_count_ty_refs from clippy_utils::ty:
| let mut ref_count = count_refs(inner_ty); | |
| let mut ref_count = peel_and_count_ty_refs(inner_ty).1 as isize; |
| while ref_count > 0 { | ||
| sugg = sugg.deref(); | ||
| ref_count -= 1; | ||
| } | ||
| while ref_count < 0 { | ||
| sugg = sugg.addr(); | ||
| ref_count += 1; | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| while ref_count > 0 { | |
| sugg = sugg.deref(); | |
| ref_count -= 1; | |
| } | |
| while ref_count < 0 { | |
| sugg = sugg.addr(); | |
| ref_count += 1; | |
| } | |
| for _ in 0..ref_count { | |
| sugg = sugg.deref(); | |
| } | |
| for _ in 0..-ref_count { | |
| sugg = sugg.addr(); | |
| } |
| ty.is_slice() | ||
| || ty.is_str() | ||
| || ty.is_diag_item(cx, sym::cstr_type) | ||
| || ty.is_diag_item(cx, sym::Path) | ||
| || ty.is_diag_item(cx, sym::OsStr) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should avoid doing multiple lookups:
| ty.is_slice() | |
| || ty.is_str() | |
| || ty.is_diag_item(cx, sym::cstr_type) | |
| || ty.is_diag_item(cx, sym::Path) | |
| || ty.is_diag_item(cx, sym::OsStr) | |
| ty.is_slice() || ty.is_str() || matches!(ty.opt_diag_name(cx.tcx), Some(sym::cstr_type | sym::Path | sym::OsStr)) |
| && [ | ||
| sym::into_boxed_str, | ||
| sym::into_boxed_slice, | ||
| sym::into_boxed_path, | ||
| sym::into_boxed_c_str, | ||
| sym::into_boxed_os_str, | ||
| ] | ||
| .contains(&second_method_path.ident.name) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Matching by name is not enough and can lead to false positives. You should ensure that this is an inherent method call and that the receiver type is the one you expect.
As an alternative, since those methods are not diag items, you could add them to clippy_utils::paths.
| if second_method_path.ident.name == sym::into_boxed_path && !inner_ty.is_diag_item(cx, sym::Path) { | ||
| // PathBuf's from(...) can convert from other str types, | ||
| // so Path::new(...) must be used to assure resulting Box is the correct type | ||
| show_lint(cx, full_span, arg, true, Some("Path::new("), "...", Some(")")); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
show_lint() should know that this suggestion should be degraded to MaybeIncorrect at most, because Path may not be have been imported.
| ) { | ||
| let mut applicability = Applicability::MachineApplicable; | ||
|
|
||
| while let ExprKind::AddrOf(BorrowKind::Ref, _, expr) | ExprKind::Unary(UnOp::Deref, expr) = inner.kind { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What if someone puts curly brackets around a subexpression, such as &{ foo.to_string() }? That would be an ExprKind::Block with one return expression, defeating this loop.
| impl<'tcx> LateLintPass<'tcx> for ClonesIntoBoxedSlices { | ||
| fn check_expr(&mut self, cx: &LateContext<'tcx>, second_method: &'tcx Expr<'_>) { | ||
| // Is the second method into_boxed_...? | ||
| if let ExprKind::MethodCall(second_method_path, first_method, _, _) = second_method.kind |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This would certainly be simplified by putting this lint into methods. The matching is basically already done there.
Adds new lint
clones_into_boxed_sliceswhich detects clone-like functions followed byinto_boxed_...instead of usingBox::from(...).Closes #15951, closes #15373
changelog: new lint: [
clones_into_boxed_slices]